Conversation
There was a problem hiding this comment.
Pull request overview
Implements the D‑NMDT and optional T‑D‑NMDT tightening formulations (Beach et al. 2024) for MIP relaxations of univariate squares (x^2) and bilinear products (x\cdot y), and adds a dedicated test suite to validate correctness and relaxation quality.
Changes:
- Added
src/quadratic_approximations/dnmdt.jlimplementing D‑NMDT for bilinear products and D‑NMDT/T‑D‑NMDT for univariate squares (via optional tightening). - Added
test/test_dnmdt_approximations.jlcovering binary expansion correctness, validity/gap bounds, tightening behavior, and comparisons vs HybS. - Wired the new implementation and tests into the package via
include(...)updates.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/quadratic_approximations/dnmdt.jl |
Adds D‑NMDT/T‑D‑NMDT modeling code, containers, and constraints for univariate and bilinear approximations. |
src/InfrastructureOptimizationModels.jl |
Includes the new DNMDT implementation file in the module load path. |
test/test_dnmdt_approximations.jl |
Adds a comprehensive test suite for DNMDT formulations (univariate, bivariate, tightening, comparisons). |
test/InfrastructureOptimizationModelsTests.jl |
Includes the new DNMDT test file in the main test runner. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| tighten::Bool = true, | ||
| epigraph_depth::Int = max(2, ceil(Int, 1.5 * depth)), | ||
| ) where {C <: IS.InfrastructureSystemsComponent} |
There was a problem hiding this comment.
epigraph_depth defaults to max(2, ceil(Int, 1.5 * depth)), but epigraph depth controls the number of tangent cuts as 2^depth + 1 (see epigraph.jl). This makes the default potentially explode in constraint count for moderate depth values and could cause big runtime/memory regressions. Consider defaulting epigraph_depth to depth (or a small constant), and/or adding a hard cap / warning in the docstring so callers don’t accidentally create tens of thousands of cuts per (name,t).
| z_con = add_variable_container!( | ||
| container, BilinearProductVariable(), C, names, time_steps; meta = meta, | ||
| ) |
There was a problem hiding this comment.
In the univariate x² approximation, the result variable container is created as BilinearProductVariable(). Reusing the bilinear-product variable type for a square approximation is likely to confuse container lookups and increases the risk of meta/key collisions if a caller builds both xy and x² approximations in the same container (especially if they reuse meta). Consider introducing a dedicated variable type for the DNMDT quadratic result (e.g., DNMDTQuadraticVariable) or avoid creating a separate variable container and store the affine expression directly, consistent with other quadratic approximation implementations.
| IOM._add_hybs_bilinear_approx!( | ||
| setup_h.container, MockThermalGen, ["dev1"], 1:1, | ||
| setup_h.x_var_container, setup_h.y_var_container, | ||
| 0.0, 1.0, 0.0, 1.0, depth, HYBS_META, | ||
| ) |
There was a problem hiding this comment.
This testset relies on HYBS_META being defined elsewhere (currently via test_hybs_approximations.jl being included earlier). That makes the file order-dependent and brittle if someone runs this test file in isolation or if the include order changes. Define const HYBS_META = "HybSTest" locally in this file (or inline the string) to remove the hidden dependency.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #39 +/- ##
==========================================
+ Coverage 22.56% 24.69% +2.12%
==========================================
Files 74 78 +4
Lines 5505 5654 +149
==========================================
+ Hits 1242 1396 +154
+ Misses 4263 4258 -5
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
@copilot update this branch on top of main |
…A, remove dead code in epigraph.jl Co-authored-by: jd-lara <16385323+jd-lara@users.noreply.github.com>
D-NMDT bilinear/quadratic approximation with optional T-D-NMDT tightening
… get_expression after
Jd/pwl documentation
renamed _container variables to something shorter and more infromative of their container type added macro for container creation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| 0.0, eps_L, 0.0, eps_L, meta * "_residual", | ||
| ) | ||
|
|
||
| # ── Epigraph tightening (T-D-NMDT only) ── |
There was a problem hiding this comment.
[JuliaFormatter] reported by reviewdog 🐶
| # ── Epigraph tightening (T-D-NMDT only) ── | |
| # ── Epigraph tightening (T-D-NMDT only) ── |
| JuMP.@variable(jump_model, base_name = "y_$(name)_$(t)") | ||
| end | ||
| return (; container, x_var_container, y_var_container, jump_model) | ||
| end No newline at end of file |
There was a problem hiding this comment.
[JuliaFormatter] reported by reviewdog 🐶
| end | |
| end | |
…A, remove dead code in epigraph.jl Co-authored-by: jd-lara <16385323+jd-lara@users.noreply.github.com>
…zationModels.jl into jd/dnmt
this is the implementation of T-D-NMDT model from 23BeachEnhancements of discretization approaches for nonconvex_part2 but keeping the tightening optional since the results in the paper aren't conclusive about performance.